Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add solution #3977

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Mihakurochkin
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request.

  1. SCSS Compilation: The HTML file links directly to an .scss file, which browsers cannot interpret. Please compile your SCSS to CSS and update the link accordingly.

  2. File Structure: The SCSS file is located in src/styles/blocks/index.scss, but it should be in src/styles/main.scss as per the task requirements.

  3. Calendar Centering: Ensure the calendar is centered both horizontally and vertically on the screen, as required by the task.

  4. Modifiers and Logic: Verify that the start-day and month-length modifiers are functioning correctly and reflect changes in the calendar layout. Also, ensure that all variables like $col-count, $cell-width, $gap, $month-length, $days-of-week, and $current-day are correctly defined and used.

  5. BEM Naming Convention: Double-check that all BEM naming conventions are correctly followed.

Once these issues are resolved, your solution will be much closer to meeting all the task requirements. Keep up the great work, and don't hesitate to reach out if you need further assistance! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

src/index.html Outdated
@@ -9,10 +9,42 @@
<title>Calendar</title>
<link
rel="stylesheet"
href="styles/index.scss"
href="./styles/blocks/index.scss"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stylesheet link references an .scss file. Browsers cannot directly interpret SCSS files. Ensure that this file is compiled to CSS and link the resulting CSS file instead.

display: flex;
gap: $gap;
flex-wrap: wrap;
width: $col-count * $cell-width + $gap * $col-count;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that the variables $col-count, $cell-width, and $gap are defined in the imported variables file. Otherwise, this calculation will not work as expected.

border: 1px solid black;

&:first-child {
margin-left: ($col-count - 1) * $cell-width + $gap * ($col-count - 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation for margin-left assumes that $col-count, $cell-width, and $gap are defined and have valid values. Verify these variables are correctly set in the imported file.

}

@each $day, $number in $days-of-week {
@if $day == $current-day {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that $days-of-week and $current-day are defined and populated correctly. Otherwise, this conditional logic will not function as intended.

}
}

@for $i from $month-length + 1 through 32 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verify that $month-length is defined and correctly represents the number of days in the current month. This loop hides days beyond the month's length.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Great job on your submission! 🎉 Your code meets the task requirements, and you've done well in structuring your SCSS and HTML files. However, there are a few critical issues that need attention:

  • SCSS Compilation: Ensure that your SCSS is compiled to CSS, as browsers cannot interpret SCSS files directly. Update your HTML to link to the compiled CSS file.

  • Variable Definitions: Double-check that variables like $col-count, $cell-width, $gap, $month-length, $days-of-week, and $current-day are correctly defined and used. These are crucial for the layout and functionality of your calendar.

  • Logic for Modifiers: Verify that the logic for modifiers such as start-day and month-length is functioning correctly. This includes ensuring that $month-length accurately reflects the number of days in the month and that the margin-left calculation for &:first-child accounts for the starting day of the week.

  • Vertical Centering: While the calendar is horizontally centered, consider implementing vertical centering for a more polished look.

Your effort is commendable, and addressing these points will enhance your code's functionality and presentation. Keep up the good work, and use this feedback to refine your skills further! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

@@ -9,10 +9,42 @@
<title>Calendar</title>
<link
rel="stylesheet"
href="styles/index.scss"
href="./styles/main.scss"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTML file links to a .scss file, which browsers cannot interpret. Please compile your SCSS to CSS and update the link to point to the compiled CSS file.

display: flex;
gap: $gap;
flex-wrap: wrap;
width: $col-count * $cell-width + $gap * $col-count;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that the variables $col-count, $cell-width, and $gap are defined in the imported variables file. This line uses these variables for calculating the width, and they must be correctly set for the layout to work as expected.

border: 1px solid black;

&:first-child {
margin-left: ($col-count - 1) * $cell-width + $gap * ($col-count - 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation for margin-left in &:first-child seems to be incorrect. It should account for the starting day of the week. Verify that the logic aligns with the intended layout.

@each $day, $number in $days-of-week {
@if $day == $current-day {
&:first-child {
margin-left: $number * $cell-width + $gap * $number;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that $days-of-week and $current-day are defined and correctly used. This logic is intended to adjust the margin based on the current day, and it must be accurate for the calendar to display correctly.

}
}

@for $i from $month-length + 1 through 32 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop for hiding days beyond the $month-length should be verified. Ensure that $month-length is set correctly to reflect the actual number of days in the month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants